Code Review

This is a solid, well-scoped fix for #6162. The root cause analysis in the PR description is accurate, and both failure modes (source-gen and reflection) are addressed. A few things worth discussing:

Source generator fix (TestMetadataGenerator.cs) ✅

Walking the base-type chain with searchType.BaseType is the correct approach here — Roslyn's GetMembers intentionally doesn't include inherited members, so this is the right fix. No issues.

Reflection fix — fragile property copying

The conversion from MethodDataSourceAttribute to InstanceMethodDataSourceAttribute manually copies two properties:

converted.Arguments = methodDataSource.Arguments;
converted.SkipIfEmpty = methodDataSource.SkipIfEmpty;

This will silently drop any new settable property added to MethodDataSourceAttribute in the future (e.g., an Order, Timeout, or SharedType equivalent). The Factory property is intentionally skipped here (it's only set by the source generator, so it's always null in reflection mode — that's correct), but the pattern is still brittle.

Suggested improvement: Add a protected or internal copy/clone method on MethodDataSourceAttribute, or a constructor overload that accepts the source attribute, so the upgrade path is self-contained and doesn't require callers to know which properties matter:

// Option A: virtual factory method on MethodDataSourceAttribute
public virtual MethodDataSourceAttribute AsInstanceVariant()
{
    var attr = ClassProvidingDataSource is { } t
        ? new InstanceMethodDataSourceAttribute(t, MethodNameProvidingDataSource)
        : new InstanceMethodDataSourceAttribute(MethodNameProvidingDataSource);
    attr.Arguments = Arguments;
    attr.SkipIfEmpty = SkipIfEmpty;
    return attr;
}

This way, when new properties are added to the base attribute they only need to be handled in one place. The current code scatters the copy logic in ReflectionAttributeExtractor, which is a poor home for it.

AmbiguousMatchException handling — misleading comment

catch (AmbiguousMatchException)
{
    // Ambiguous overloads - leave the attribute as-is and let runtime resolution handle it
    return false;
}

The comment says "let runtime resolution handle it" — but returning false skips the InstanceMethodDataSourceAttribute conversion, so if the overloaded method is an instance member, the runtime will hit the exact Activator.CreateInstance failure this PR was written to fix. The comment creates false confidence that the skip is safe.

Possible options:

1. If overloaded instance data source methods are not a supported use case, change the comment to say so explicitly.
2. If they should work, iterate over targetType.GetMethods(flags).Where(m => m.Name == memberName) and return true if any overload is non-static (conservative: if any overload is instance, treat it as instance).

At minimum the comment should be updated to: // Overloaded name — cannot determine statically; may fail at runtime if the member is an instance method.

Duplicated BindingFlags constant

The comment // Must stay in sync with MethodDataSourceAttribute.BindingFlags highlights a maintenance risk. Since MethodDataSourceAttribute.BindingFlags is private, the extractor has to duplicate it. If they ever diverge, the pre-check in TargetsInstanceMember would disagree with what GetDataRowsAsync resolves at runtime.

Suggested improvement: Promote the constant to internal in MethodDataSourceAttribute and reference it directly from ReflectionAttributeExtractor, eliminating the sync requirement.

Test coverage ✅

The regression test in TUnit.TestProject/Bugs/6162/Tests.cs accurately reproduces the reported scenario (abstract base with instance property data source, [InheritsTests], DI-constructed derived class with no parameterless constructor). It's well-structured and clearly documented.

Summary

The fix is correct and the overall approach is right — mirroring the source generator's compile-time conversion in reflection mode at the single choke point in BuildTestMetadata is a clean design. The three issues above are worth addressing, with the property-copy fragility being the most architecturally significant. Happy to approve once the AmbiguousMatchException comment is at least updated to be accurate.

Code Review: fix/6162-inherited-instance-method-data-source

This is a clean, well-targeted bug fix with accurate root-cause analysis and good test coverage. The dual-mode fix is correctly structured — source-gen and reflection paths are each fixed independently, which is the right approach.

Source Generator Fix (TestMetadataGenerator.cs)

The change from GetMembers to a base-type walk is correct — Roslyn's ITypeSymbol.GetMembers(name) is declared-members-only, so the inherited member was invisible before. The fix is minimal and focused. Taking members[0] is consistent with the pre-existing FirstOrDefault() behaviour.

One minor point: if someone has overloaded methods with the same name where some are static and some instance (contrived, but possible), only members[0] is checked for the isStatic decision. This was already the case before this PR, and the source generator later emits both the type and method name into the Factory lambda, so the right overload is ultimately resolved. Not a regression, just worth a note.

Reflection Fix (ReflectionAttributeExtractor.cs)

The TargetsInstanceMember design is sound. Using Type.GetMember with BindingFlags.Instance | FlattenHierarchy correctly traverses the full inheritance chain for instance members — instance members are already traversed by the Instance flag, and FlattenHierarchy additionally pulls in inherited static members. The conservative approach (any matching instance member → treat as instance-targeting) is the right call for overload ambiguity.

The and not InstanceMethodDataSourceAttribute guard in ExtractMethodDataSources correctly skips already-upgraded attributes, and since ExtractDataSources returns a new array, the in-place mutation is safe for concurrency.

ToInstanceVariant()

All user-settable state (Arguments, SkipIfEmpty) is copied. Factory is correctly excluded — it is always null in reflection mode since it is source-generator-populated. The internal visibility is appropriate.

Public API Snapshots

The d__21 → d__22 state-machine number bump is expected: the new ToInstanceVariant method shifts the compiler-generated async state machine number for GetDataRowsAsync. The snapshot updates are correct.

Regression Test

The repro in TUnit.TestProject/Bugs/6162/Tests.cs precisely targets the reported failure scenario (abstract base with an instance property data source, sealed subclass constructed via DI with no parameterless constructor). [EngineTest(ExpectedResult.Pass)] ensures it runs in both modes.

Overall, this is a solid, well-reasoned fix. No issues found. Good work.

Code Review — Round 3

Both items raised in previous reviews (property-copy fragility, AmbiguousMatchException risk) have been addressed cleanly in the follow-up refactor commit.

Previous feedback — confirmed addressed

Property-copy fragility → ToInstanceVariant() on the attribute itself
Moving the conversion onto MethodDataSourceAttribute is the right call. New settable properties only need handling in one place, next to the type that owns them. The explicit Factory exclusion with inline documentation is clear and correct — it is always null in reflection mode.

AmbiguousMatchException risk → Type.GetMember enumeration
GetMember with the shared BindingFlags constant handles overloads without throwing. The conservative "any instance member → treat as instance-targeting" approach is appropriate. The BindingFlags constant is correctly promoted to internal and referenced from both sites rather than duplicated.

Current state

Everything looks good:

• The base-type walk in the source-gen path uses Roslyn's declared-members-only GetMembers, which is the correct API to iterate explicitly.
• ExtractMethodDataSources guards with and not InstanceMethodDataSourceAttribute before upgrading — no double-conversion possible.
• Arguments in ToInstanceVariant() is a reference copy rather than a clone, but attribute argument arrays are set once at construction and never mutated in this codebase, so this is acceptable.
• The public API snapshot changes (d__21 → d__22) are an expected compiler artefact from the new internal method shifting the async state machine ordinal — not a public surface change.
• Regression test precisely targets the reported failure path (abstract base + instance property data source + sealed derived class with no parameterless constructor + DI data source).

No further concerns. Solid fix.